fix(desktop): stop agent config reverts from stale-store laundering - #3752
Open
wpfleger96 wants to merge 6 commits into
Open
fix(desktop): stop agent config reverts from stale-store laundering#3752wpfleger96 wants to merge 6 commits into
wpfleger96 wants to merge 6 commits into
Conversation
The retention cache resolved an equal-`created_at` collision by skipping the inbound event unconditionally, while the relay breaks that tie by lowest event id. The two could therefore disagree about which same-second event is the head, and every later disk-vs-head compare inherited the error. Retained rows now carry the event id as a real column so the local compare can match `replace_parameterized_event`. A pending row still wins its tie regardless of id: it is durable local intent, arbitrated against a writer-consistent head by the boot pass rather than dropped by a cache compare. The id is re-derived from a parsed and verified event, never read from the JSON's self-asserted `id` field, so a legacy row whose bytes do not verify stays unresolved instead of receiving a key it could win a tie with. `RetainedEvent::pending`/`::inbound` replace the hand-built literals at every call site, which makes it structurally impossible for a row's ordering key to describe different bytes than its own `raw_event`. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
A pending row is durable local intent — an edit this device made and has not yet published. Resolving inbound events by timestamp let a strictly newer event clear `pending_sync` and patch `personas.json`, destroying that edit. Timestamps are not evidence of newer intent: the laundering vector this change targets re-signs stale content at `max(now, head + 1)`, so every laundered revert arrives strictly newer. That made the untrusted input the one that always won. The pending check now precedes every ordering rule, and the new `Deferred` outcome keeps "lost the compare" distinct from "awaiting arbitration" for the boot decision pass. Callers gate on `Applied` so neither non-apply outcome reaches disk. Until that pass lands a pending row shadows genuinely newer remote edits for its coordinate; flush normally clears it within seconds, and an edit that can still be reconciled is worth more than one that cannot be recovered. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Boot reconcile re-signs whatever is on disk at `monotonic_created_at = max(now, head + 1)`, so a merely-stale store mints events strictly newer than the edits they revert. Last-write-wins then adopts the revert. A second install with a fresh retention database is exactly this state, which is why a dev build silently reverts config edited elsewhere. `created_at` cannot arbitrate it: the value the stale store carries is manufactured by the reconcile, not observed. The barrier arbitrates on PROVENANCE instead. A baseline records what this install last agreed was published at a coordinate; with no baseline, the install cannot show it ever agreed to anything there, so neither its content nor its timestamps are evidence of intent about the relay's state. `decide` runs four ordered gates: positive tombstone evidence, then the no-baseline park, then the queued-deletion timestamp compare, then the queued edit against the baseline. The ordering is load-bearing — the timestamp compare is sound only because the no-baseline gate already excluded every store that would win it spuriously. Both publication gates are enforced, because a coordinate's record and its queued tombstone occupy different primary keys and the flush loop reads them independently: gating one row computes a suppression and enforces half of it. The live gate is default-deny over the decision set, so a variant added later is withheld until deliberately admitted. The scan is per-owner and unfiltered by `#a` because the relay post-filters `#a` after its `LIMIT`, so absence is claimed only from a page shorter than the limit it asked for. Nothing is applied to disk here. `ApplyHead`, `RestoreFromRelay`, and `DeleteLocal` are decided, logged, and gated; resolving them is the user-driven step. On relay deployments with a replica-routed read pool, the head lookups and tombstone scan read best-available rather than writer-consistent; staleness within replication lag is a documented known limitation (see `head_lookup` and `tombstone_scan` module docs). The barrier's per-decision tracing line is the detection mechanism if it fires. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
wpfleger96
force-pushed
the
duncan/agent-config-revert-fix
branch
from
July 30, 2026 22:19
b29f86f to
cd8f244
Compare
Add a per-scope readiness latch (config_sync_ready_scope in AppState)
that the flush loop checks before publishing any row. Before this, the
flush loop ran independently of the boot barrier, so legacy pending rows
from a prior boot were publishable at t≈0 — before reconcile or barrier
had run — recreating the stale-store revert this PR exists to close.
Changes:
- Add publish_blocked field to RetainedEvent and carry it in all query
sites (get_pending_sync, get_retained_event, get_retained_personas,
legacy_migration reader). New rows start publish_blocked: false;
the barrier sets it via set_publish_blocked.
- Add config_sync_ready_scope: Mutex<Option<PathBuf>> to AppState.
Starts None. Set to Some(db_path) by run_boot_barrier on successful
completion; never set on any error path.
- flush_active_pending_events checks readiness before snapshotting
pending rows. If not ready it runs run_boot_barrier inline, then
re-checks: a barrier failure skips this tick and retries on the next,
so a transient relay-unreachable at boot does not wedge publishing
for the session.
- flush_pending_events_at re-reads publish_blocked immediately before
submit_signed_event_at_with_keys. A row gated after the pending
snapshot was taken cannot publish.
- apply_workspace clears config_sync_ready_scope to None before
spawn_event_sync so a workspace switch lands unready until its own
barrier passes.
- Remove the 'in practice' timer-luck paragraph from event_sync.rs;
replace with the enforced structural invariant.
- Remove all writer-consistent and sync_authoritative references from
desktop/src-tauri; replace with exact, best-available,
replica-lag-exposed language. Grep acceptance: empty.
- Add three deterministic tests in flush_barrier module:
(a) test_row_gated_after_snapshot_cannot_submit: barrier gates a row
after the pending snapshot; flush sees publish_blocked on re-read
and skips it; row stays pending.
(b) test_barrier_error_leaves_scope_unready: latch starts None, stays
None until barrier succeeds, cleared on workspace switch.
(c) test_retry_path_publishes_after_barrier_succeeds: latch None →
scope not ready; latch set → flush proceeds and row publishes.
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Replace map_or(true, ...) with is_none_or(...) per clippy lint unnecessary_map_or (-D warnings gate). Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…pth under latch Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Agent system prompts, models, and thinking/effort settings revert silently when more than one Buzz build shares an identity against the same relay. Agent configs sync as NIP-33 replaceable events (
kind:30175/30176/30177) resolved last-write-wins oncreated_at, and every desktop boot re-signs whatever its local store holds atmonotonic_created_at = max(now, head + 1). A stale store therefore launders old content into a winning head at every launch.This PR fixes both halves: the ordering primitive that cleans up the retention cache, and the boot decision pass that stops the laundering before it can publish.
Part 1 — Ordering
Pending local intent outranks ordering
pending_sync = 1marks durable local intent — an edit the user made that this device has not published yet.retain_inbound_eventnow checks for a pending row before any timestamp or event-id comparison, and returns a distinctDeferredoutcome that mutates nothing.A newer
created_atis not evidence of newer intent. The laundering vector mints events atmonotonic_created_at = max(now, head + 1), so every laundered revert arrives strictly newer than the edit it reverts. Resolving inbound events by timestamp alone therefore handed the win to precisely the untrusted input: a strictly newer event clearedpending_syncand patchedpersonas.json, destroying the unpublished edit before any arbitration could see it.Callers in
commands/personas/inbound.rsgate onApplied, so neither non-apply outcome reaches disk.Deferredstays distinct fromSkippedso the decision pass can tell "lost the ordering compare" from "still owes arbitration."The ordering bug
For non-pending rows, the retention cache and the relay disagreed about which of two same-second events is the head.
replace_parameterized_event(crates/buzz-db/src/lib.rs) breaks an equalcreated_atby lowest event id — it rejects an incoming event whencreated_at == accepted_ts && incoming_id >= accepted_id. The desktop cache skipped every equal-timestamp inbound event instead. Nostr timestamps are seconds-granularity, so this collision is reachable in normal use, and once the cache held a head the relay disagreed with, every later disk-vs-head comparison inherited the error.retain_inbound_eventnow applies the relay's own rule to non-pending rows.Trusting the ordering key
event_idis stored as a real column rather than reparsed fromraw_eventon every compare, and it is always derived from an event that parsed and cryptographically verified — never read from the JSON's self-assertedidfield. A legacy row whose stored bytes do not verify keepsevent_id IS NULLand is treated as unorderable: the retained row stands and nothing is decided from a fabricated key.RetainedEvent::pending/::inboundreplace the hand-built struct literals at all 13 call sites. Every event-derived field is read from the single event passed in, so a row built through them cannot carry anevent_iddescribing different bytes than its ownraw_event.Schema migration
baseline_event_idandbaseline_contentare added alongsideevent_id; they carry the provenance the decision pass reads. The migration is additive and crash-safe. A cheap read-onlypragma_table_infoprobe decides whether anything is missing; only then does it open aBEGIN EXCLUSIVEtransaction, re-probe inside the lock, and apply theALTER TABLEs together with theevent_idbackfill. Two processes opening the same database concurrently are safe — the loser waits on the write lock, re-probes, and finds nothing to do.Part 2 — Boot decision pass
Root cause
The laundering vector exists because boot reconcile re-signs the stale store's content at
monotonic_created_at, minting events that are definitionally newer than the edits they revert.created_atcannot arbitrate this: the value is manufactured by the reconcile, not observed. No ordering rule can fix it.The barrier arbitrates on provenance instead. A baseline records what this install last agreed was published at a coordinate. With no baseline, the install cannot show it ever agreed to anything there, so neither its content nor its timestamps are evidence of intent about the relay's state — the pending row must be withheld until the user resolves the conflict or a future boot with a baseline can arbitrate cleanly.
Decision table (
decide)Four ordered gates:
Park— withhold, surface conflictPublishDeletionorDeferPublish,StampBaseline, orDeferGate ordering is load-bearing: gate C's timestamp compare is sound only because gate B already excluded every store that would win it spuriously (the manufactured-timestamp path).
Both publication gates are enforced independently, because a coordinate's live record and its queued tombstone occupy different primary keys and the flush loop reads them independently. The live gate is default-deny over the decision set, so a variant added later is withheld until deliberately admitted.
Evidence
{kinds, authors, #d, limit:2}queries. A transport failure becomesLookupFailed, neverAbsent, so a flaky network cannot masquerade as a deletion.#ais post-filtered server-side, so absence is claimed only from a page shorter than the limit. The scan runs once per boot and yields evidence for all coordinates in one pass.Composed regression (Will's bug)
test_stale_store_with_no_baseline_cannot_revert_a_newer_headandtest_no_baseline_store_withholds_both_the_record_and_its_tombstoneexercise the exact scenario: a second install, no baseline, stale disk, live head. Gate B parks it; the flush loop suppresses both the live record and its tombstone row.The composed regression path runs through the real
run_decision_pass/gate enforcement path, not a mock, verifying the fulldecide → apply_gate → get_pending_synccomposition.Known limitations
On relay deployments that route historical reads through a read-replica pool, head lookups and the tombstone scan read best-available rather than writer-consistent. A replica that has not yet replayed a write answers absence in the dangerous direction. The exposure window equals replica lag (normally single-digit seconds) and requires the coincidence of an agent-config write followed immediately by a boot-time barrier read from behind the lag. The barrier's per-decision tracing line surfaces this if it fires in the wild. Re-adding a writer-pinned read opt-in to the protocol is a small standalone relay PR reversible on observed evidence.
Deferred
Explicit conflict resolution surface (Step 6 / V3.6) — follow-up work, not in this PR: a conflict UI backed by a
get_blocked_coordinatesquery and push-local / accept-remote commands.A coordinate parked by the barrier (
publish_blocked = 1) is a stable fixed point:retain_event's upsert does not touchpublish_blocked, and each subsequent boot re-parks the same coordinate so long as no baseline exists. An intentional edit made on an install with no prior publish history for that agent stays local-only and unpublished until the resolution surface ships. This is the correct polarity for dev-build collisions (the scenario this PR closes), but it also affects a legitimate second install where the user has edited an agent for the first time on that machine.The support path today is the barrier's per-decision tracing line (
buzz::config_sync), which logs everyParkdecision with coordinate, kind, and reason, and direct SQL against the retention DB. The follow-up delivers a conflict UI with push-local / accept-remote actions, clearingpublish_blockedand stamping a baseline atomically per coordinate.